-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feature #3796 arrayMinItems.populate = never #3809
feature #3796 arrayMinItems.populate = never #3809
Conversation
@@ -96,6 +96,11 @@ const liveSettingsSelectSchema: RJSFSchema = { | |||
title: 'Ignore minItems unless field is required', | |||
enum: ['requiredOnly'], | |||
}, | |||
{ | |||
type: 'string', | |||
title: 'Ignore populating arrays even if settled minItems', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit confusing. I'm not sure what settled minItems
refers to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you will help me with how to describe it properly?
I tried to describe that your formData
won't be populated by any default values of the array:
schema:
...
"anyArray": {
"type": "array",
"minItems": 1,
"items": { "type": "string" }
}
...
formData:
{
"anyArray": []
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the legacy behavior choice says Populate remaining minItems with default values
I think the following makes sense.
title: 'Ignore populating arrays even if settled minItems', | |
title: 'Never populate minItems with default values', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
expect( | ||
computeDefaults(testValidator, schema, { | ||
rootSchema: schema, | ||
rawFormData: { nonRequiredArray: ['raw0'] }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might I suggest a different value than the default so that we can see which value is actually being chosen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course. Which one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rawFormData: { nonRequiredArray: ['raw0'] }, | |
rawFormData: { nonRequiredArray: ['raw1'] }, |
Which may require the expected value below to be updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@alekseyBatuhtin Can you resolve the conflicts in the |
@@ -96,6 +96,11 @@ const liveSettingsSelectSchema: RJSFSchema = { | |||
title: 'Ignore minItems unless field is required', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And to make the text consistent across all choices, please also make this change
title: 'Ignore minItems unless field is required', | |
title: 'Only populate minItems with default values when field is required', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
cb0c2c2
to
577d27f
Compare
CHANGELOG.md
Outdated
@@ -15,6 +15,18 @@ it according to semantic versioning. For example, if your PR adds a breaking cha | |||
should change the heading of the (upcoming) version to include a major version bump. | |||
|
|||
--> | |||
# 5.13.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this can be moved into the 5.12.0
section below since we haven't released it yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Reasons for making this change
Hi team! I've already described the problem in the issue below:
#3796
Checklist
npm run test:update
to update snapshots, if needed.